Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow references to casks when running upgrade and outdated #7927

Merged
merged 16 commits into from
Jul 27, 2020
Merged

Allow references to casks when running upgrade and outdated #7927

merged 16 commits into from
Jul 27, 2020

Conversation

whoiswillma
Copy link
Member

@whoiswillma whoiswillma commented Jul 6, 2020

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew tests with your changes locally?

Tests were not written because I think the same conditions hold for this PR as for this other PR

brew upgrade upgrades both formulae and casks.

brew outdated was tricky to integrate because of the --json option. It makes sense for brew outdated with no arguments to print all outdated formulae and casks, which implies brew outdated --json should print the json for formulae and casks. However, I did not want to change the behavior of brew outdated --json lest any scripts that depend on it break. I settled on the following compromise:

  1. brew outdated --json prints json information for only formulae, which makes it equivalent to brew outdated --json --formula-only
  2. brew outdated --json --cask-only prints json information for only casks.

Another possibility is a v2 json API for brew outdated, but I haven't seen any demand for such feature

A small script to install an outdated version of atom and youtube-dl (cask and formula respectively). Useful for testing
echo "Uninstalling atom, youtube-dl"
brew uninstall atom youtube-dl

LR=$pwd

echo "Installing atom 1.47.0"
cd /usr/local/Homebrew/Library/Taps/homebrew/homebrew-cask/
git checkout a023e3d0754c0d8a5647891328f5b64caa5512ba -- Casks/atom.rb
brew cask install atom
git restore --staged Casks/atom.rb
git restore Casks/atom.rb

echo "Installing youtube-dl 2020.05.08"
cd /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/
git checkout 877666acfa7af926b0623dcdc7839e182c8e95d4 -- Formula/youtube-dl.rb
brew install youtube-dl
git restore --staged Formula/youtube-dl.rb
git restore Formula/youtube-dl.rb

cd $LR

@whoiswillma whoiswillma marked this pull request as draft July 6, 2020 23:24
@MikeMcQuaid
Copy link
Member

brew upgrade upgrades both formulae and casks.

@homebrew/cask @vitorgalvao any objections/thoughts on this?

@miccal
Copy link
Member

miccal commented Jul 7, 2020

How would this work the the unique options for brew upgrade and brew cask upgrade?

For brew upgrade:

Screen Shot 2020-07-07 at 21 38 01

For brew cask upgrade:

Screen Shot 2020-07-07 at 21 36 55

@MikeMcQuaid
Copy link
Member

@miccal It would accept arguments for both and only pass on those relevant to the given command.

@whoiswillma
Copy link
Member Author

whoiswillma commented Jul 7, 2020

The relevant flags passed to brew upgrade are forwarded to brew cask upgrade, and --greedy has been added as a flag to brew upgrade for the sole purpose of interacting with brew cask upgrade. No errors are shown when a brew upgrade-only switch is used (although I think there is a usability argument for this).

I guess Mike and I were writing the same comment at the same time, oops :/

@miccal
Copy link
Member

miccal commented Jul 7, 2020

Thank you for the explanation @whoiswillma and @MikeMcQuaid -- I think it would make sense to unify the upgrade commands for brew and brew cask.

@vitorgalvao
Copy link
Member

any objections

None.

@whoiswillma whoiswillma marked this pull request as ready for review July 7, 2020 15:00
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One tweak otherwise looking great!

Library/Homebrew/cmd/outdated.rb Outdated Show resolved Hide resolved
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there! Nice work 👏

Library/Homebrew/cmd/outdated.rb Outdated Show resolved Hide resolved
Library/Homebrew/cmd/outdated.rb Outdated Show resolved Hide resolved
Library/Homebrew/cmd/outdated.rb Outdated Show resolved Hide resolved
Library/Homebrew/cmd/outdated.rb Outdated Show resolved Hide resolved
Library/Homebrew/cmd/upgrade.rb Outdated Show resolved Hide resolved
@jonchang
Copy link
Contributor

@whoiswillma can you enable edits for maintainers? This pull request needs to be rebased; I've done it locally but it needs to be on GitHub before this can be merged. Thanks!

@whoiswillma
Copy link
Member Author

Seems like GitHub is being a bit finicky with push access on PRs since I'm not the one that made the fork. I think you have direct write access to the fork now. Is that enough?

@jonchang
Copy link
Contributor

Thanks, looks like that worked.

@reitermarkus
Copy link
Member

I think for the --json flag we should default to --json=v1 and print both formulae and casks with --json=v2.

@reitermarkus
Copy link
Member

Maybe also deprecate --json without explicit version.

@whoiswillma
Copy link
Member Author

I'll work on those changes

@whoiswillma
Copy link
Member Author

whoiswillma commented Jul 16, 2020

For reference, here is an example output of json for outdated formula:

[
  {
    "name":"libgcrypt",
    "installed_versions":[
      "1.8.5"
    ],
    "current_version":"1.8.6",
    "pinned":false,
    "pinned_version":null
  }
]

and outdated casks:

[
  {
    "name":"atom",
    "installed_versions":"1.47.0",
    "current_version":"1.49.0"
  }
]

Given that the schema is slightly different between formulae and casks, I think the json v2 for formulae and casks should be completely separate. Maybe something like:

{
    "casks": ...
    "formulae": ...
}

But I can see the argument for having them be merged: thoughts?

@whoiswillma
Copy link
Member Author

whoiswillma commented Jul 17, 2020

Commit history is a bit messed up. I'll work on fixing it tomorrow

Update: Commit history is fixed now.

Library/Homebrew/cmd/outdated.rb Outdated Show resolved Hide resolved
Library/Homebrew/cmd/outdated.rb Outdated Show resolved Hide resolved
Library/Homebrew/cmd/outdated.rb Outdated Show resolved Hide resolved
Library/Homebrew/cmd/outdated.rb Outdated Show resolved Hide resolved
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good so far, nice work!

Library/Homebrew/cmd/outdated.rb Outdated Show resolved Hide resolved
Library/Homebrew/cmd/outdated.rb Show resolved Hide resolved
Library/Homebrew/cmd/outdated.rb Outdated Show resolved Hide resolved
Library/Homebrew/cmd/outdated.rb Outdated Show resolved Hide resolved
Library/Homebrew/cmd/outdated.rb Outdated Show resolved Hide resolved
Library/Homebrew/cmd/outdated.rb Outdated Show resolved Hide resolved
Library/Homebrew/cmd/outdated.rb Outdated Show resolved Hide resolved
Library/Homebrew/cmd/outdated.rb Outdated Show resolved Hide resolved
Library/Homebrew/cmd/outdated.rb Outdated Show resolved Hide resolved
Library/Homebrew/cmd/outdated.rb Outdated Show resolved Hide resolved
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there 🎉

Library/Homebrew/cmd/outdated.rb Outdated Show resolved Hide resolved
Library/Homebrew/cmd/outdated.rb Outdated Show resolved Hide resolved
Library/Homebrew/cmd/outdated.rb Outdated Show resolved Hide resolved
@MikeMcQuaid MikeMcQuaid merged commit 2ff56c9 into Homebrew:master Jul 27, 2020
@MikeMcQuaid
Copy link
Member

Thanks again @whoiswillma!

@miccal
Copy link
Member

miccal commented Aug 4, 2020

@whoiswillma and @MikeMcQuaid I have created an issue here that I believe is a consequence of the changes made in this PR.

Thank you.

@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Dec 20, 2020
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Dec 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants